Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve fallback #236

Closed
wants to merge 2 commits into from
Closed

Conversation

crissdev
Copy link
Contributor

Closes #210

@rubenv Let me know if I should add more tests. The implementation is pretty straightforward.

@rubenv
Copy link
Owner

rubenv commented Oct 27, 2015

Pretty much what I had in mind!

One more needed change: it needs to use the fallback language in getPlural: a plural form lookup for ru_RU will return the default (English) version, instead of what the ru rules define.

@rubenv
Copy link
Owner

rubenv commented Oct 27, 2015

And with that in mind, it's probably better to name it baseLanguage or similar.

As in "Dutch is the base language for Dutch (Netherlands)". :-)

@crissdev
Copy link
Contributor Author

Some notes about the changes:

  • baseLanguage is private because it conflicts with catalog's attribute with the same name. I noticed in the docs it's now deprecated. I think with a 3.0 release this method can be public and replace the attribute.
  • plural detection is pretty hard-coded (see the if statement in the getPlural method - there's no clear way to know if plural is defined or not since for every language that's not defined a default value is returned. To make things even difficult a plural form is defined for pt_BR. A solution would be to change the generated output to allow more control. Any thoughts on this?

- baseLanguage is private because it conflicts with catalog's attribute with the same name
- plural detection is pretty hardcoded, there's no clear way to know if plural is defined or not
@CrlsMrls
Copy link

CrlsMrls commented Dec 8, 2015

+1 please merge :)

@rubenswieringa
Copy link
Contributor

@crissdev / @rubenv what's the status of this? Also, should there maybe be a test that checks that, when a language and its fallback both have a translation, the former its translation is returned?

@rubenv
Copy link
Owner

rubenv commented Dec 16, 2015

Had a quick look at this (thanks for the ping @rubenswieringa).

One way to make this cleaner is to add an extra function getStringFormFor(lang, string, n, context) (suggestions for a better name welcome). This could then be used by getStringForm():

getStringForm: function (string, n, context) {
    return this.getStringFormFor(this.currentLanguage, string, n, context) ||
           this.getStringFormFor(this.baseLanguage(), string, n, context);
}

The call to gettextPlurals would then have to move from getPlural to inside getStringFormFor, so that each language can have different rules.

This would reduce the need to hardcode pt_BR and it wouldn't require changes to gettextPlurals.

A fallback here would amount to:

// pt_BR    Brazilian Portuguese  nplurals=2; plural=(n > 1);
// pt       Portuguese            nplurals=2; plural=(n != 1);
// Assuming n = 0
return this.getStringFormFor("pt_BR", string, n, context) || // gettextPlurals("pt_BR", n) -> 0
       this.getStringFormFor("pt", string, n, context);      // gettextPlurals("pt", n) -> 1

Does that make sense?
Anyone want to take this up?

@rubenswieringa
Copy link
Contributor

@rubenv I'm looking into it, if I manage to come up with anything I'll open up a new PR.

@rubenswieringa
Copy link
Contributor

The call to gettextPlurals would then have to move from getPlural to inside getStringFormFor,

@rubenv are you sure we can move that function-call from getPlural() to getStringFormFor()?

When getStringForm() gets called, it expects that its n-parameter is a value that has already been modified by gettextPlurals(). Then it would call getStringFormFor(), which would also call gettextPlurals(). So gettextPlurals() would be called twice – corrupting the value of n.

Are my assumptions correct?

@rubenv
Copy link
Owner

rubenv commented Dec 18, 2015

When getStringForm() gets called, it expects that its n-parameter is a value that has already been modified by gettextPlurals().

I was implying that we should change this behavior, but since getStringForm() is external API, we can't right?

Either way: any value of n to my hypothetical getStringFormFor() would indeed need to be the unmodified n coming from the app.

@rubenswieringa
Copy link
Contributor

So I suppose getStringForm() and getPlural() can't share a method that they both call then?

@rubenv
Copy link
Owner

rubenv commented Dec 18, 2015

Well, I don't think getStringForm() is used all that much. If getting good fallback language support comes at the expense of breaking getStringForm(): no problem, I'm willing to bump to a new version for this. The only main purpose of getStringForm() is to make getString() and getPlural() work: those are the ones that matter.

@rubenswieringa
Copy link
Contributor

Alright, so do you want to completely get rid of getStringForm()?

@rubenv
Copy link
Owner

rubenv commented Dec 18, 2015

We can always add it back if people complain, but given the way fallback support works, it probably doesn't even make sense to have a method that behaves like the current getStringForm().

It'll be slightly annoying for the few people that depend on it, but we'll be saving them from pluralization bugs in Portuguese :-)

rubenswieringa pushed a commit to rubenswieringa/angular-gettext that referenced this pull request Dec 19, 2015
…ew function

Removes getStringForm(). See discussion here: rubenv#236 (comment)
@rubenswieringa
Copy link
Contributor

New PR: #250

@rubenv
Copy link
Owner

rubenv commented Dec 19, 2015

Cool, let's close this one.

@rubenv rubenv closed this Dec 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants